Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract EditorDockManager from EditorNode #84193

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Oct 30, 2023

Moves most code about docks, dock management, and dock select popup from EditorNode to EditorDockManager.
There should be no behavioral changes in this pr.

Details

  • Created new class EditorDockManager singleton to encapsulate docks and dock select logic from EditorNode
  • EditorNode passes the dock slots and dock splits to EditorDockManager
  • Created DockSplitContainer : SplitContainer that automatically shows if any children are visible, otherwise it hides
  • EditorNode still adds the Scene, FileSystem, and other docks but it uses EditorDockManager to do it.
  • Made save_docks_to_config, load_docks_from_config, update_dock_slots_visibility public in EditorDockManager to be accessible from EditorNode.
  • Made close_all_floating_docks as part of the workaround for Godot crashes when changing Editor Features when certain docks are floating #84125. This can be removed after the proper fix, if wanted.
  • Added signal layout_changed in EditorDockManager to trigger EditorNode::_save_editor_layout(). This could probably be changed to EditorNode::get_singleton()->save_editor_layout_delayed() if the delay is fine.
  • EditorPlugin and VersionControlEditorPlugin now use EditorDockManager to add and remove docks
  • Added optional name parameter to add_control_to_dock, since the default docks use a custom title on the tabs. This is not exposed to users.
  • Changed some parameter names to be better (ofs -> p_offset, p_which -> p_dock_slot)

@kitbdev kitbdev requested a review from a team as a code owner October 30, 2023 14:36
@YuriSizov YuriSizov added this to the 4.3 milestone Oct 30, 2023
@AThousandShips AThousandShips added bug crash cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release enhancement and removed enhancement bug crash cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Oct 30, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Oct 30, 2023

  • EditorDockManager is a MarginContainer that is the second child of main_vbox (under the title bar) and it handles the SplitContainers under that. It exposes its main_hsplit so EditorNode can handle the center panel and bottom panel.

Why is it a node at all?

There is no reason for this. It should only handle the docks themselves, and if you insert it as a node and as a node in this particular place, it automatically becomes a parent to not only docks, but also to the main editor and the bottom panel.

I think docks and dockable splits should be made into a reusable control, and a manager class can be introduced to handle them without becoming a key node in the editor layout tree.

Edit: This allows us to have a modular approach when it comes to how the layout works. This will be important if we want to make floating windows into mini editors with their own dockable areas.

@kitbdev
Copy link
Contributor Author

kitbdev commented Oct 30, 2023

I only made it a node so it can take all of the SplitContainers and dock slots from EditorNode, without needing to pass each of them to the manager.

I was going to look into doing godotengine/godot-proposals#1984 later, but I like your modular idea so I'll look into that.

@kitbdev kitbdev marked this pull request as draft October 30, 2023 14:59
@YuriSizov
Copy link
Contributor

YuriSizov commented Oct 30, 2023

I only made it a node so it can take all of the SplitContainers and dock slots from EditorNode, without needing to pass each of them to the manager.

I have plans to completely divorce the UI part of the editor from EditorNode, there would be an EditorLayout node or something, and it can handle registering dockable areas and what not. Then this kind of layout node would also be used by floating windows.

But that shouldn't affect your PR much. I just don't think that making a dock manager a root of most UI is correct for its functionality.

Edit: By the way, feel free to tag me on RocketChat if you want to figure out a solution together!

@kitbdev kitbdev force-pushed the dock-manager-extract branch from 9de5339 to 3d2d34a Compare October 31, 2023 01:57
@trollodel
Copy link
Contributor

Cool. I actually wanted to do this after #62378, but I didn't find the time.
What's the status of this?

@kitbdev
Copy link
Contributor Author

kitbdev commented Dec 8, 2023

@trollodel after #84122 is merged, I'll rebase this and then I think it will be ready for review

@YuriSizov YuriSizov self-requested a review December 8, 2023 11:18
@YuriSizov
Copy link
Contributor

You can rebase this now and we can look into getting it across the finish line 🙂

editor/editor_node.h Outdated Show resolved Hide resolved
@kitbdev kitbdev force-pushed the dock-manager-extract branch from 3d2d34a to 9224e0c Compare December 8, 2023 17:43
@kitbdev kitbdev marked this pull request as ready for review December 8, 2023 17:50
@kitbdev kitbdev requested a review from a team as a code owner December 8, 2023 17:50
@kitbdev
Copy link
Contributor Author

kitbdev commented Dec 8, 2023

Rebased and updated initial comment to be accurate to the changes. The Dock Manager is now an Object and the dock slots are passed into it from the EditorNode. The Dock Manager still handles all of the dock select Controls. Added DockSplitContainer to handle automatically showing and hiding the SplitContainer.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 14, 2024

This is how it looks in master, I thought it was intentional?

No, this is how it looks on master:

image

The space between 1st and 2nd element is smaller, but it's there. Though it makes it look more obviously like a bug.

@kitbdev
Copy link
Contributor Author

kitbdev commented Jan 14, 2024

No, this is how it looks on master:

Hmm, this is how master looks for me:
image
Godot v4.3.dev (26b1fd0) - Windows 10.0.22621 - Vulkan (Forward+)

Looking at _dock_select_draw(), I didn't change anything, so this is weird.

Edit:
Looks like there is only 1px of spacing, so the resolution matters a lot and it looks to be inconsistent based on where it is.
UI Scale set to 75%:
image
UI Scale set to 125%:
image

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
EditorDockManager::EditorDockManager() {
singleton = this;

dock_select_popup = memnew(PopupPanel);
Copy link
Contributor

@YuriSizov YuriSizov Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This popup (with all of its layout) could probably be extracted into its own class, it contains plenty of code which doesn't need to be shared.

You also won't need to connect to theme_changed this way, you will be able to use the notification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, this can probably be done in a follow-up.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes are necessary, but overall this is a very clean and welcome extraction. Great job!

@YuriSizov
Copy link
Contributor

Regarding the drawing of the popup the code seems identical to me. But it's not great to begin with, it doesn't account for the scale, I think. So glitches are possible, and I'm pretty sure I've seen it misbehave like that before. It needs a lot of improvements, really, but I don't think this PR breaks it.

@kitbdev kitbdev force-pushed the dock-manager-extract branch from 91f5082 to 0db8642 Compare January 15, 2024 19:50
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, aside from the missing empty line.

@kitbdev kitbdev force-pushed the dock-manager-extract branch from 0db8642 to 2323f04 Compare January 15, 2024 20:15
@akien-mga akien-mga merged commit 039379d into godotengine:master Jan 16, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants